kvcoord: avoid piggybacking on any early-returning batch #151916
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
stevendanna
force-pushed
the
ssd/txn-wb-split-batch
branch
from
58864dc to
cba48b8
Compare
| // TestBatchHeaderFieldsAreAccountedForInBufferedWrites checks that any new | ||
| // fields added to the batch header are appropriately accounted for by the | ||
| // needed functions. | ||
| func TestBatchHeaderFieldsAreAccountedForInBufferedWrites(t *testing.T) { |
There was a problem hiding this comment.
@miraradeva What do you think about this as a way to try to keep this from drifting over time?
There was a problem hiding this comment.
Yeah, seems reasonable. These types of tests have def saved me before. Hurray for unit tests as documentation.
stevendanna
force-pushed
the
ssd/txn-wb-split-batch
branch
2 times, most recently
from
f95e66b to
fe24e41
Compare
stevendanna
changed the title
To reduce the number of batches, we'd _like_ to piggy-back our buffer flush on the user-provided batch. But, in some cases this is problematic as the user may have set options on the batch that can result in the batch being incompletely processed. Here, we check for those options and flush the buffer via a sanitized batch before processing the user's batch. In a follow-up, we may also _always_ use a separate batch for midTxn flushes to avoid nearly all read-write batches. Epic: None Release note: None
Informs #Fixes Epic: none Release note: None
stevendanna
force-pushed
the
ssd/txn-wb-split-batch
branch
from
fe24e41 to
5e2a418
Compare
|
Test failure looks unrelated. |
There was a problem hiding this comment.
@miraradeva reviewed 2 of 4 files at r1, 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @stevendanna)
pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 1774 at r3 (raw file):
midTxnFlush := !hasEndTxn splitBatchRequired := separateBatchIsNeeded(ba) // || midTxnFlush
nit: remove comment?
pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 1819 at r3 (raw file):
clearBatchRequestOptions(flushBatch) flushBatch.Requests = reqs log.VEventf(ctx, 2, "flushing %d buffered requests via separate batch", len(reqs))
Is it useful to log the full batch, in case we need to debug header field values?
To reduce the number of batches, we'd like to piggy-back our buffer
flush on the user-provided batch. But, in some cases this is problematic
as the user may have set options on the batch that can result in the
batch being incompletely processed. Here, we check for those options and
flush the buffer via a sanitized batch before processing the user's
batch.
In a follow-up, we may also always use a separate batch for midTxn
flushes to avoid nearly all read-write batches.
Epic: None
Release note: None